r? @onur-ozkan

rustbot has assigned @onur-ozkan.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Some changes occurred in src/tools/compiletest

cc @jieyouxu

Makes sense, thanks!

@bors r+ rollup

📌 Commit 03373ee has been approved by onur-ozkan

It is now in the queue for this repository.

Just in case, have you benchmarked the time to execute the UI suite between the two executors?

With libtest:

$ x test ui -- --force-rerun -N

test result: ok. 18603 passed; 0 failed; 322 ignored; 0 measured; 0 filtered out; finished in 187.32s
test result: ok. 18603 passed; 0 failed; 322 ignored; 0 measured; 0 filtered out; finished in 193.96s
test result: ok. 18603 passed; 0 failed; 322 ignored; 0 measured; 0 filtered out; finished in 189.54s

With the new executor:

$ x test ui -- --force-rerun

test result: ok. 18603 passed; 0 failed; 322 ignored; 0 measured; 0 filtered out; finished in 195.85s
test result: ok. 18603 passed; 0 failed; 322 ignored; 0 measured; 0 filtered out; finished in 195.76s
test result: ok. 18603 passed; 0 failed; 322 ignored; 0 measured; 0 filtered out; finished in 195.69s

Based on a few local runs, the new executor seems to consistently be a couple of seconds slower across a 3 minute ui test run, which is a little surprising to me since it should be doing marginally less work overall.

It's interesting that the new executor seems to be more stable. 5% slower isn't that terrible, but it would be nice to investigate more why it seems to be slower.

Bug found in deadline detection (#139660 (comment)); I'll push a fix.

@bors r-

Let me also do a few smoke runs of the new executor on msvc in case anything funny occurs.

I'd also mark this as rollup-never since we're changing the tests/ test harness itself.
@bors rollup=never

Maybe there's another issue with the deadline logic (with the now <= condition fixed): given a test

// tests/ui/foo.rs

//@ run-pass

fn main() {
    std::thread::sleep(std::time::Duration::from_secs(70)); // 70 to hit the libtest default slow test warning threshold of 60s
}

Due to the way how the libtest-esque test filtering logic works, ./x test foo will run 4 tests.

• With the libtest executor, I observe only 1 warning correspond to this synthetic slow test

  running 4 tests
  i..test [ui] tests/ui/foo.rs has been running for a long time
  .
  

• With the new executor, I observe warnings for all the tests

  running 4 tests
  i..test [ui] tests/ui/cfg/cfg-macros-foo.rs has been running for a long time
  test [ui] tests/ui/cfg/cfg-macros-notfoo.rs has been running for a long time
  test [ui] tests/ui/foo.rs has been running for a long time
  test [ui] tests/ui/missing_non_modrs_mod/foo.rs has been running for a long time
  

Yeah, I'm seeing the same thing.

I think it's because we don't actually remove completed tests from the deadline queue when they finish, so tests are treated as having timed out long after they have already finished. This was being masked by the other bug, which made us not emit any timeout events due to the backwards check.

I wonder if we can come up with some kind of self-tests for this 🤔

Well the good news is that fixing the deadline bugs also seems to eliminate the perf difference. 😅

I think what I want to do here is open a separate PR to fix the deadline bugs (hopefully with some unit tests), and then we can revisit this PR once that has landed.

Sounds good

Separate PR to fix deadline bugs: #140031.